feat(node): signed proxy refs with file handler#370
Conversation
📝 WalkthroughWalkthroughThis PR adds a signed cross-SDK proxy/reference mechanism to the Node SDK: a new ChangesCross-SDK Proxy System
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
sdks/node/src/proxies/canonical.ts (1)
35-41: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExotic objects (Date, Map, Set, RegExp) silently canonicalize as
{}instead of throwing.The generic
typeof value === "object"branch (Line 35) catches anything that isn't an array, includingDate,Map,Set,RegExp, etc.Object.entries()on these yields no own enumerable properties, so they all silently canonicalize to"{}"rather than hitting the final "must be JSON-serializable" throw at Line 42. Since this is the deterministic signing form for the cross-SDK HMAC contract, any futureProxyHandlerthat puts such a value into areferencefield would get a canonical form that discards the actual value's identity — two differentDates would sign identically.Consider guarding for plain objects only (e.g. checking the prototype is
Object.prototypeornull) and throwing otherwise, consistent with the primitive rejection already in place.🛡️ Proposed fix to reject non-plain objects
if (typeof value === "object") { + const proto = Object.getPrototypeOf(value); + if (proto !== Object.prototype && proto !== null) { + throw new ProxyError( + `proxy reference values must be plain JSON-serializable objects, got ${value?.constructor?.name ?? typeof value}`, + ); + } const entries = Object.entries(value as Record<string, unknown>) .filter(([, v]) => v !== undefined) .sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0)) .map(([k, v]) => `${JSON.stringify(k)}:${canonicalJson(v)}`); return `{${entries.join(",")}}`; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/node/src/proxies/canonical.ts` around lines 35 - 41, The canonicalJson object branch in canonical.ts is too broad and currently treats exotic objects like Date, Map, Set, and RegExp as plain objects, which makes them serialize to "{}" instead of failing. Tighten the typeof value === "object" path in canonicalJson to accept only plain objects (for example by checking the prototype is Object.prototype or null) and route all other object types to the existing non-JSON-serializable rejection, so ProxyHandler reference values cannot lose identity during signing.sdks/node/src/index.ts (1)
34-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor style inconsistency: split type/value exports.
Other modules in this file combine type and value exports from the same specifier in one statement (e.g. Line 2:
export { type DashboardAuth, type DashboardOptions, serveDashboard } from "./dashboard";). The proxies exports are split into two separateexport/export typestatements from the same"./proxies"module instead.♻️ Optional consolidation for consistency
-export type { ProxyHandler, ProxyRef } from "./proxies"; -export { canonicalJson, FileProxyHandler, FileReference, Proxies } from "./proxies"; +export { + canonicalJson, + FileProxyHandler, + FileReference, + Proxies, + type ProxyHandler, + type ProxyRef, +} from "./proxies";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/node/src/index.ts` around lines 34 - 35, The proxies exports in index.ts are split across separate type and value statements, unlike the other combined exports in this file. Update the "./proxies" re-exports to match the existing style by consolidating ProxyHandler and ProxyRef with the value exports in a single export statement, keeping the same symbols (ProxyHandler, ProxyRef, canonicalJson, FileProxyHandler, FileReference, Proxies) and preserving their type/value distinctions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@sdks/node/src/index.ts`:
- Around line 34-35: The proxies exports in index.ts are split across separate
type and value statements, unlike the other combined exports in this file.
Update the "./proxies" re-exports to match the existing style by consolidating
ProxyHandler and ProxyRef with the value exports in a single export statement,
keeping the same symbols (ProxyHandler, ProxyRef, canonicalJson,
FileProxyHandler, FileReference, Proxies) and preserving their type/value
distinctions.
In `@sdks/node/src/proxies/canonical.ts`:
- Around line 35-41: The canonicalJson object branch in canonical.ts is too
broad and currently treats exotic objects like Date, Map, Set, and RegExp as
plain objects, which makes them serialize to "{}" instead of failing. Tighten
the typeof value === "object" path in canonicalJson to accept only plain objects
(for example by checking the prototype is Object.prototype or null) and route
all other object types to the existing non-JSON-serializable rejection, so
ProxyHandler reference values cannot lose identity during signing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 961c5f9e-869d-40e0-b1de-dc1b2e0f5252
📒 Files selected for processing (9)
sdks/node/README.mdsdks/node/src/errors.tssdks/node/src/index.tssdks/node/src/proxies/canonical.tssdks/node/src/proxies/file-handler.tssdks/node/src/proxies/index.tssdks/node/src/proxies/proxies.tssdks/node/src/proxies/types.tssdks/node/test/resources/proxies.test.ts
Summary
Adds a proxies module to the Node SDK — the last missing module in the SDK feature matrix. Explicit model: producers deconstruct a non-serializable resource into a signed
ProxyRefcarried in the payload; workers verify and reconstruct it. No argument walking.src/proxies/:Proxiesregistry (register/deconstruct(value, { ttlMs, purpose })/reconstruct(ref, expectedPurpose?)/resolve<T>),ProxyHandler<T>SPI,ProxyReftype,FileReference+FileProxyHandler, andcanonicalJson.handlerId \n canonicalJson(reference) \n expiry \n purpose, where an absent expiry signs as the literal string"null"and an absent purpose as"". A golden contract-vector test pins the exact signature so drift breaks the build. Verification is timing-safe (with the length pre-checktimingSafeEqualrequires), expiry is strict, and purpose binding is enforced on request.canonicalJsonis a hand-rolled recursive serializer (sorted keys by UTF-16 code units, compact,undefinedvalues dropped) — object round-tripping would reorder integer-like keys numerically. Non-safe-integer numbers are rejected because their textual form is not stable across the contract; decimals go as strings.FileProxyHandlerenforces an optional allowlist by real (symlink-resolved) path: the nearest existing ancestor is realpath'd, the remainder re-appended, and containment checked segment-aware — a symlink inside an allowed root pointing outside it is rejected.ProxyError extends TaskitoError; wire refs carry explicit JSON nulls for the optional fields, so the types acceptnull | undefined.Tests
19 tests in
test/resources/proxies.test.ts: wire round-trip, tampered ref/expiry, allowlist accept/reject + symlinked-ancestor escape, no-handler/unknown-handler/null-value/duplicate-id/empty-key rejections, TTL round-trip + expiry, purpose enforcement, registration-order dispatch, canonical-JSON units (recursive sort, lexical integer-like keys, undefined handling, float rejection), and the golden contract vector. Full suite: 248 passed.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests